Skip to content

feat: use TypeScript directly for Deno runtime, skip bundling#386

Merged
netanelgilad merged 4 commits intomainfrom
claude/issue-385-20260309-0658
Mar 17, 2026
Merged

feat: use TypeScript directly for Deno runtime, skip bundling#386
netanelgilad merged 4 commits intomainfrom
claude/issue-385-20260309-0658

Conversation

@netanelgilad
Copy link
Contributor

@netanelgilad netanelgilad commented Mar 9, 2026

Note

Description

Since Deno supports TypeScript natively, this PR changes the build pipeline to copy deno-runtime/main.ts directly to dist/assets/deno-runtime/main.ts instead of bundling it into a JavaScript file. This removes an unnecessary Bun build step for the Deno runtime, simplifying the build pipeline and avoiding potential bundling artifacts. All references to the Deno runtime asset are updated from .js to .ts accordingly.

Related Issue

Closes #385

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Other (please describe):

Changes Made

  • infra/build.ts: Added copyDenoRuntime() helper that uses copyFileSync/mkdirSync to copy deno-runtime/main.ts as-is to dist/assets/deno-runtime/main.ts, replacing the previous runBuild() step; updated watch-mode and production build log output to show "Copied" instead of "Rebuilt" for the Deno runtime
  • infra/build-binaries.ts: Updated required asset path check from dist/assets/deno-runtime/main.js to main.ts
  • src/core/assets.ts: Updated getDenoWrapperPath() to return the .ts extension instead of .js

Testing

  • I have tested these changes locally
  • I have added/updated tests as needed
  • All tests pass (npm test)

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (if applicable)
  • My changes generate no new warnings
  • I have updated docs/ (AGENTS.md) if I made architectural changes

Additional Notes

The Deno runtime is executed directly by the Deno binary, which has native TypeScript support, making bundling to JS redundant. This change keeps the source TypeScript intact in the distributed assets and removes one Bun build invocation from the pipeline.


🤖 Generated by Claude | 2026-03-10 00:00 UTC

Since Deno supports TypeScript natively, copy deno-runtime/main.ts as-is
instead of bundling it to JS. This removes an unnecessary build step and
simplifies the pipeline.

- Replace Bun bundle step in infra/build.ts with fs.copyFileSync
- Update getDenoWrapperPath() to reference main.ts instead of main.js
- Update build-binaries.ts prerequisite check accordingly

Co-authored-by: Netanel Gilad <netanelgilad@users.noreply.github.com>
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2026

🚀 Package Preview Available!


Install this PR's preview build with npm:

npm i @base44-preview/cli@0.0.41-pr.386.7892f13

Prefer not to change any import paths? Install using npm alias so your code still imports base44:

npm i "base44@npm:@base44-preview/cli@0.0.41-pr.386.7892f13"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "base44": "npm:@base44-preview/cli@0.0.41-pr.386.7892f13"
  }
}

Preview published to npm registry — try new features instantly!

@netanelgilad
Copy link
Contributor Author

@claude are the changes here covered by tests? the tests run on the real dist output with the deno runtime we copied? or is the change not really covered and I need to test it manually?

@claude
Copy link

claude bot commented Mar 9, 2026

Claude finished @netanelgilad's task in 1m 0s —— View job


Test Coverage Analysis for Deno Runtime Changes

  • Read changed files
  • Investigate test suite for Deno runtime coverage
  • Check if tests run against real dist output

Short answer: these changes are NOT covered by automated tests. You'll need to test manually.

Here's the breakdown:

What the testkit does

CLITestkit.ensureTestAssets() (tests/cli/testkit/CLITestkit.ts:264) does copy dist/assets/ into a temporary ~/.base44/ directory before each test run — so the tests genuinely exercise the real dist output including whatever is in dist/assets/deno-runtime/. That part is solid.

Why it doesn't cover this PR

The only consumer of getDenoWrapperPath() is src/cli/commands/dev.ts:17, which passes it to createDevServer(). There is no tests/cli/dev.spec.ts (or any equivalent) — the dev command has zero test coverage. So:

  1. src/core/assets.ts change (.js.ts path) — not exercised by any test
  2. infra/build.ts change (copyDenoRuntime() replacing runBuild()) — build infra is not unit tested
  3. infra/build-binaries.ts change (checking for main.ts) — same, no tests for binary build verification

What needs manual testing

To verify this PR works correctly, you'd need to:

  1. Run bun run build and confirm dist/assets/deno-runtime/main.ts exists (not main.js)
  2. Run base44 dev in a project with functions and confirm the Deno runtime starts correctly from the .ts file
  3. Optionally run bun run build:binaries and check the asset check passes with main.ts
    |

@netanelgilad
Copy link
Contributor Author

@claude merge main (might be shallow clone) and fix any conflicts after the changes that happened in the main

@claude
Copy link

claude bot commented Mar 9, 2026

Claude finished @netanelgilad's task in 1m 24s —— View job


Merged main into PR branch

  • Analyze request
  • Fetch latest main (unshallow if needed)
  • Merge origin/main into current branch
  • Fix conflict in packages/cli/infra/build-binaries.ts
  • Push changes

Main had a monorepo migration (#372) that moved everything under packages/cli/. There was one conflict in infra/build-binaries.ts (now packages/cli/infra/build-binaries.ts) where both sides had changed the deno-runtime asset path — main still had main.js while this PR had updated it to main.ts. Kept main.ts as that's the intent of this PR.

All three PR changes are now correctly in their new monorepo locations:

  • packages/cli/infra/build.tscopyDenoRuntime() copies main.ts
  • packages/cli/infra/build-binaries.ts — checks for main.ts
  • packages/cli/src/core/assets.ts — returns .ts path

github-actions bot and others added 3 commits March 9, 2026 12:12
Resolved conflict in infra/build-binaries.ts by keeping the PR's
`main.ts` change while adopting the monorepo file layout from main.

Co-authored-by: Netanel Gilad <netanelgilad@users.noreply.github.com>
@kfirstri kfirstri moved this from Backlog to In progress in CLI Development Mar 16, 2026
@artemdemo
Copy link
Contributor

@claude review

@claude
Copy link

claude bot commented Mar 17, 2026

Claude finished @artemdemo's task in 1m 20s —— View job


Code Review

  • Read changed files
  • Analyze diff
  • Post review feedback

Overall: LGTM — clean, correct refactor. The change is well-scoped and the reasoning is sound. Deno has native TypeScript support, so the previous bundling step was redundant.


packages/cli/infra/build.ts

copyDenoRuntime() function (build.ts:28-33) — Clean and minimal. Uses mkdirSync(..., { recursive: true }) which is safe to call even if the dir already exists. ✓

One observation: copyDenoRuntime() uses relative ./ paths, which requires the build to be invoked from the packages/cli/ directory. This is consistent with how the existing runBuild() calls work (entrypoints: ["./src/cli/index.ts"], outdir: "./dist/cli"), so no issue there.

Watch mode (build.ts:78): The watch list correctly includes ./deno-runtime so changes to main.ts will trigger copyDenoRuntime(). ✓


packages/cli/infra/build-binaries.ts

Prerequisite check (build-binaries.ts:56): Updated from main.jsmain.ts. Straightforward. ✓


packages/cli/src/core/assets.ts

getDenoWrapperPath() (assets.ts:17): Returns main.ts path. This is consumed by dev.ts:17 and eventually passed to function-manager.ts:129:

spawn("deno", ["run", "--allow-all", this.wrapperPath], { ... })

deno run accepts .ts files natively, so passing the .ts path works correctly. ✓


Potential future concern (not a blocker)

deno-runtime/main.ts is currently self-contained (no local imports). If it ever grows to import from sibling files (e.g., utilities), those files would need to be copied to dist/assets/deno-runtime/ as well — copyDenoRuntime() only copies main.ts. Worth keeping in mind as the Deno runtime evolves, but not an issue today.


As noted in a prior comment, the base44 dev flow has no automated test coverage, so this should be validated manually: run bun run build and confirm dist/assets/deno-runtime/main.ts exists, then run base44 dev in a project with functions to confirm Deno starts correctly.

Copy link
Contributor

@artemdemo artemdemo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no objections

@github-project-automation github-project-automation bot moved this from In progress to In review in CLI Development Mar 17, 2026
@netanelgilad netanelgilad merged commit 8c2cdcb into main Mar 17, 2026
10 checks passed
@netanelgilad netanelgilad deleted the claude/issue-385-20260309-0658 branch March 17, 2026 10:17
@github-project-automation github-project-automation bot moved this from In review to Done in CLI Development Mar 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Use ts for deno runtime, no bundle

3 participants